feat(web): simplify message delivery controls - #1314
Draft
mindfn wants to merge 5 commits into
Draft
Conversation
Why: users need one understandable delivery choice, a truthful whole-conversation Stop, and an explicit destructive Steer confirmation instead of overlapping cancellation semantics.
Why: a failed durable stop request must not leave the conversation appearing stopped without feedback.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f81b884278
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? dispositionCarrierSupport === 'exact' | ||
| ? (messageDisposition.oneShot ?? undefined) | ||
| : 'next_work' | ||
| ? (messageDisposition.oneShot ?? messageDisposition.effective) |
There was a problem hiding this comment.
When active work first appears and the preference GET is still pending or has failed, messageDisposition.effective remains the local next_work default. Sending that value explicitly gives it precedence in resolveMessageDispositionForAdmission, so a persisted global continue_current preference is silently ignored and the message is queued. Send only the one-shot override explicitly, or wait until preference hydration succeeds before sending the effective value.
Useful? React with 👍 / 👎.
| // #1307: a conversation-level Stop is always a full-thread stop. Per-cat | ||
| // cancellation remains an internal control-plane primitive and Steer uses | ||
| // its dedicated, explicitly destructive path. | ||
| cancelFn(threadId, undefined); |
There was a problem hiding this comment.
When the new force-reset POST rejects or returns a non-2xx response, cancelInvocation only displays an error toast, but handleStop immediately proceeds to clear callbacks, active invocation slots, and streaming state. The server invocation therefore continues while the UI removes its liveness indicators and Stop control, potentially also dropping subsequent output until a refresh or rehydration. Return and await the request result, clearing local state only after success or restoring it on failure.
Useful? React with 👍 / 👎.
Why: delivery preference belongs with append custody; this PR now contains only the destructive Stop and Steer interaction.
Why: a rejected whole-thread Stop must leave the visible invocation state intact so users can retry or inspect the failure.
Collaborator
Author
|
@codex review the latest head; prior inline findings were addressed in subsequent commits. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de0db64c4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // its dedicated, explicitly destructive path. | ||
| const clearStoppedThread = () => { | ||
| clearPendingCallbacksForThread(threadId); | ||
| const isActiveThreadStop = threadId === store.currentThreadId; |
There was a problem hiding this comment.
When a user stops foreground thread A and navigates to an already-running thread B before the force-reset request resolves, store is the Zustand snapshot captured before the request, so this comparison still treats A as active. The success callback then calls active-thread helpers such as clearAllActiveInvocations(), which operate on the store's current thread and clear B's running/streaming UI instead of only A. Read the current state after the await (or always perform thread-scoped cleanup) before selecting the cleanup branch.
Useful? React with 👍 / 👎.
Why: a force-reset can finish after navigation; re-reading the store prevents the prior thread's Stop from clearing the newly active thread's running state.
Collaborator
Author
|
@codex review the latest head; this addresses the newly reported cross-thread async Stop P1. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
mindfn
marked this pull request as draft
August 7, 2026 09:34
mindfn
added a commit
to mindfn/clowder-ai
that referenced
this pull request
Aug 12, 2026
Why: preserve the accepted zts212653#1312 confirmation flow while zts212653#1314 standardizes whole-thread Stop semantics on the fork integration base.
mindfn
added a commit
to mindfn/clowder-ai
that referenced
this pull request
Aug 12, 2026
* feat(web): simplify message delivery controls Why: users need one understandable delivery choice, a truthful whole-conversation Stop, and an explicit destructive Steer confirmation instead of overlapping cancellation semantics. * fix(web): report whole-thread stop failures Why: a failed durable stop request must not leave the conversation appearing stopped without feedback. * refactor(web): keep whole-thread Stop independently scoped Why: delivery preference belongs with append custody; this PR now contains only the destructive Stop and Steer interaction. * fix(web): retain running state when Stop fails Why: a rejected whole-thread Stop must leave the visible invocation state intact so users can retry or inspect the failure. * fix(web): scope async Stop cleanup to current thread Why: a force-reset can finish after navigation; re-reading the store prevents the prior thread's Stop from clearing the newly active thread's running state. * test(web): cover confirmation-based Steer in stop integration Why: preserve the accepted zts212653#1312 confirmation flow while zts212653#1314 standardizes whole-thread Stop semantics on the fork integration base.
mindfn
added a commit
to mindfn/clowder-ai
that referenced
this pull request
Aug 12, 2026
* chore(fork): prompt authority upgrade + LL-054 hotfix lesson Squashed fork-specific customizations: - Remove hardcoded identity from AGENTS.md/GEMINI.md (fix CLI system-level prompt overriding Cat Café identity injection) - Add LL-054: don't modify governance chain without analyzing existing mechanism Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(skills): harden skill gates — prevent repeated rework failure mode Squashed 5 skill gate hardening commits (codex R1-R3 review fixes): - quality-gate: add Patch Counter + upstream ref validation - receive-review: refine trigger conditions - request-review: gate hardening - writing-plans: plan validation gate - refs/review-request-template: add review template Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore(fork): add cost discipline rules — agent demotion + debug gate Why: Usage analysis showed 93% cost from subagent-heavy sessions. Add §20 to shared-rules (agent demotion reflex, debug strategy gate, session cost awareness) and a Cost Discipline summary to CLAUDE.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(F192): publish-verdict works in fork checkout — explicit gh --repo + no overlapping-publish retries (#11) * fix(F192): publish-verdict works in fork checkout — explicit gh --repo + no overlapping-publish retries Two bugs blocked cat_cafe_publish_verdict in the fork checkout (砚砚 diagnosed 2026-06-17; the eval cats have been unable to publish any verdict for days): git-worktree-publisher.ts pushes the verdict branch to `origin` (mindfn/clowder-ai) but every `gh` call relied on cwd auto-detection. This checkout has BOTH `origin` (fork) AND `upstream` (zts212653/clowder-ai), and `gh` resolves the base repo to the upstream parent. So `gh pr create --head <branch>` looked for the branch in zts212653 and failed with "Head sha can't be blank / No commits between main and branch / Head ref must be a branch". (Same failure I hit manually early in this feature and worked around with `--repo`.) Worse latent bug: the failure-cleanup `gh pr list` probe in `finally` also auto-detected upstream — it would see no PR for the pushed branch, decide `safeToDelete`, and `git push --delete origin <branch>`, orphaning a live PR's source branch. Fix: derive `owner/repo` from `git remote get-url origin` and pass an explicit `--repo <owner/repo>` to ALL gh invocations (label create, pr create, pr close, pr list). In a single-remote upstream checkout this is a no-op (origin IS the repo); in a fork it pins the correct target. `parseOwnerRepoFromGitRemoteUrl()` is exported + unit-tested across scp, ssh://, https, cred-embedded, and trailing-slash URL forms. The MCP callback layer uses a 10s per-attempt fetch timeout + [1s,2s,4s] retry. The publish route runs a synchronous git worktree + push + gh pr create (~17s typical). Result: the client aborts at 10s but the SERVER keeps running, and the 3 auto-retries each kick off a fresh server-side publish — 4 overlapping publishes racing on the same `verdict/auto/<domain>/<id>` branch (砚砚 saw 4 incoming /publish-verdict in the logs). The caller never gets the PR URL. Bumping the global env timeout is wrong (every callback would then wait that long on a hung socket). Fix: thread an optional per-call `fetchTimeoutMs` + `retryDelaysMs` override through callbackPost → sendCallbackRequest → postJsonWithRetry (all additive, backward-compat), and have publish-verdict-tool.ts pass `fetchTimeoutMs: 180_000` + `retryDelaysMs: []` (single attempt, no retry). Server-side idempotency guards (verdict_already_exists / branch-exists) remain the real safety net for "did it publish". - parseOwnerRepoFromGitRemoteUrl: all URL forms + defensive throws - postJsonWithRetry: retryDelaysMs=[] → exactly 1 attempt; fetchTimeoutMs override widens the bound; no-override still honors the env default - existing publisher cleanup tests + 30 publish-verdict pipeline tests still green; mcp-server callback suite 60/60 [宪宪/opus-4.8🐾] * fix(F192): derive origin repo before mkdtempSync — no temp-dir leak (砚砚 P3) 砚砚 review P3 on PR #11: the origin lookup (`git remote get-url origin` → parseOwnerRepoFromGitRemoteUrl) sat AFTER mkdtempSync but outside the try/finally. A missing/invalid `origin` would throw after the temp dir was created, leaking it (the finally cleanup is never entered). Pure reorder — move the fallible origin derivation ABOVE mkdtempSync so it throws before any side effect. Code now matches its own comment ("failing before any side effect is the safe outcome"). No logic change. Publisher tests still 4/4 green. [宪宪/opus-4.8🐾] * fix(mcp-server): remove duplicate PUBLISH_VERDICT_FETCH_TIMEOUT_MS declaration Two fork commits each added a module-level const with the same name (unused 120_000 at top of file, actually-used 180_000 near handlePublishVerdict) — TS2451 redeclare error, broke the MCP server build after rebuild-onto-main. Also picks up a pre-existing Biome formatting fix in callback-outbox.ts that was blocking the commit gate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(sidebar): 统一所有 Tab 的会话排序规则 (#93) ## Summary 所有 sidebar tab (最近/置顶/项目/系统/收藏) 和项目组内会话复用同一个 comparator： pinned → active-run → unread → lastActiveAt ↓ → createdAt ↓ → id。 删除了 sortPinnedUnreadTitle（项目/系统/收藏的标题序排序），统一为活跃度排序。 项目组间 active/archive 分组逻辑不变。 Fixes: zts212653#1305 (排序统一部分) ## Evidence - selector 58/58; sidebar/component 88/88 - pnpm check, web production build, full build, tsc --noEmit 全部通过 - Cross-family review: 布偶猫 APPROVE @ 69d2b59 Merge-Owner: 布偶猫/宪宪 (claude-opus-4-6) Author: 缅因猫/sol (gpt-5.6-sol) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(F192): recover evidence-prereq gate lost in develop_base rebuild (#91) Cherry-pick recovery of PR #27 (3384026). The F192 evidence-source prerequisite gate was merged to develop_base on 2026-07-11 but lost when develop_base was rebuilt from main during the F257 main-sync. Restores createTelemetryEvidencePrereqProbe which checks OTel availability at boot — when disabled, the daily eval fires a zero-LLM-cost skip notice instead of invoking the eval cat. Review: codex APPROVE at exact HEAD 2f7348b with 0 findings. Evidence: 12/12 node:test pass, API build clean, biome+brand guard pass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(eval): preserve public verdict publication without census (#96) Why: the public export intentionally strips the home-only F267 census, but the runtime loaded and refreshed it unconditionally, blocking even keep_observe verdicts. Treat absence as keep-observe-only while retaining the actionable validity gate and census-aware behavior when the registry exists. [砚砚/gpt-5.6-sol🐾] Thread-Context: threadId=thread_eval_friction catId=codex-sol * docs(f167): record session strategy semantic drift Why: repeated operator corrections exposed an implementation-anchoring failure that conflated policy intent, telemetry availability, and lifecycle action capability. * docs(f167): capture cross-thread scope escalation Why: a strategy discussion was incorrectly promoted to a blocking requirement for an in-flight PR, so the evidence must distinguish notification urgency from scope authority. * feat(messages): reliable queued delivery, recall/edit, and scoped retry (#94) * feat(messages): make append delivery traceable Why: expose durable append evidence and make failed target delivery safely retryable without resending user messages. * fix(messages): preserve retry custody under concurrency Why: repeated body exposure and rejected stale retries must not corrupt delivery history or leave dead retry controls. * fix(messages): preserve server defaults and cancelled retries Why: a stale client preference must not override persisted delivery intent, and a stopped invocation should remain recoverable without reopening author withdrawals. * fix(messages): scope retry to selected target Why: retrying one failed recipient must not re-run sibling targets or rewrite their attempt history; receipt assertions now preserve the durable attempt contract. * fix(messages): retain target-scoped retries Why: retrying one failed recipient must not redeliver to its siblings or let F175 batch unrelated queued work into the scoped attempt. * fix(messages): roll back rejected retry scope Why: a rejected durable retry fence must restore both failure evidence and the target dispatch scope, or an unauthorised retry can run later. * fix(messages): preserve retry scope through recovery Why: target-scoped retries must survive durable queue recovery and cannot be merged into a normal multi-target dispatch. * test(messages): cover both retry batching directions Why: retain explicit regressions for scoped retries as both a batch primary and a batch candidate. * refactor(messages): remove superseded withdrawal projection Why: the rebased #94 now uses the current custody settlement source of truth; remove the conflict-era duplicate path so recall and withdrawal cannot diverge. * fix(messages): terminalize withdrawn target attempts Why: author withdrawal must close active delivery attempts as source_withdrawn while keeping only invocation-cancelled attempts retryable. * fix(web): scope whole-thread Stop lifecycle safely (#97) * feat(web): simplify message delivery controls Why: users need one understandable delivery choice, a truthful whole-conversation Stop, and an explicit destructive Steer confirmation instead of overlapping cancellation semantics. * fix(web): report whole-thread stop failures Why: a failed durable stop request must not leave the conversation appearing stopped without feedback. * refactor(web): keep whole-thread Stop independently scoped Why: delivery preference belongs with append custody; this PR now contains only the destructive Stop and Steer interaction. * fix(web): retain running state when Stop fails Why: a rejected whole-thread Stop must leave the visible invocation state intact so users can retry or inspect the failure. * fix(web): scope async Stop cleanup to current thread Why: a force-reset can finish after navigation; re-reading the store prevents the prior thread's Stop from clearing the newly active thread's running state. * test(web): cover confirmation-based Steer in stop integration Why: preserve the accepted zts212653#1312 confirmation flow while zts212653#1314 standardizes whole-thread Stop semantics on the fork integration base. * feat(sessions): make manual seal safe during stop teardown (#95) * feat(sessions): add safe manual seal Why: let users rotate idle Session Chains after configuration changes without interrupting live work or losing history. * fix(sessions): block sealing every live turn Why: session integrity depends on the cat/thread being idle, not on matching the browser caller identity. * fix(sessions): atomically fence manual session seals Why: manual sealing must exclude a racing invocation and must not claim success when transcript or digest finalization is partial. * fix(sessions): refresh after partial seal Why: a manual seal can complete before transcript finalization returns a 503, so the panel must reconcile its active card with the authoritative session chain while preserving the actionable error. * fix(invocation): block manual seal while cancel tombstone teardown pending (zts212653#1313) Stop→immediately Seal raced: cancel() tombstone made has() return false, so guardSessionSeal() acquired the guard while the provider route was still tearing down — risking concurrent transcript flush / incomplete seal. Add teardownComplete flag to canceled tombstones. guardSessionSeal() now also checks hasPendingTeardown(): a canceled slot blocks seal until complete()/completeSlot()/completeAll() marks teardown done. Expired tombstones and new-start purges are unaffected. has() stays false for tombstones (queue gates unchanged). resolveFinalStatus() still sees the tombstone after teardown completes. 12 targeted regression tests (red→green TDD). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(invocation): cancelAll tombstones slots instead of deleting (zts212653#1313 P1) Terra review: Stop button fires force-reset → cancelAll() which deleted slots outright. guardSessionSeal() then saw empty map, acquired the guard, and proceeded with seal while provider teardown was still running. Change cancelAll to tombstone (state='canceled') instead of delete, matching cancel()'s behaviour. guardSessionSeal() now sees hasPendingTeardown()=true and blocks until complete/completeSlot/completeAll marks teardownComplete. Invariants preserved: - has() returns false for tombstones (queue gates, tryStartThread unaffected) - batch.aborted still drives resolveFinalStatus (not tombstone) - getActiveSlots excludes tombstones - start/startAll replaces tombstones normally - TTL expiry backstop works 9 new regression tests for cancelAll→Seal race (red→green TDD). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix(invocation): fence scoped preempt teardown Why: manual sealing must wait for route cleanup after a force preempt, without blocking unrelated dispatches or replacement starts. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * test(messages): retain stop-teardown fence in force race Why: force replacement cancels a multi-cat batch, and #95 deliberately retains non-primary canceled tombstones until route teardown completes. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes #1307.
Verification
Known non-blocking